Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(linkace): set fsGroup to www-data (82) #3050

Merged
merged 2 commits into from
Jul 2, 2022

Conversation

schnerring
Copy link
Contributor

@schnerring schnerring commented Jul 2, 2022

Description

Starting up Linkace currently causes 500 internal server errors because the web process is unable to write to the log directory.

⚙️ Type of change

  • ⚙️ Feature/App addition
  • 🪛 Bugfix
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 🔃 Refactor of current code

🧪 How Has This Been Tested?

Setting fsGroup to 82 through Web GUI fixes 500 errors. See also: Kovah/LinkAce#332

📃 Notes:

One issue still remaining is that .env is not writable during the setup process. The user has to use the container shell to chmod 777 .env to be able to finish the setup. I think this is an upstream issue, but is there maybe an easy fix we can implement?

image

✔️ Checklist:

  • ⚖️ My code follows the style guidelines of this project
  • 👀 I have performed a self-review of my own code
  • #️⃣ I have commented my code, particularly in hard-to-understand areas
  • 📄 I have made corresponding changes to the documentation
  • ⚠️ My changes generate no new warnings
  • 🧪 I have added tests to this description that prove my fix is effective or that my feature works
  • ⬆️ I increased versions for any altered app according to semantic versioning

@schnerring schnerring requested a review from stavros-k as a code owner July 2, 2022 19:01
@truecharts-admin truecharts-admin added the size/XS Categorises a PR that changes 0-9 lines, ignoring generated files. label Jul 2, 2022
@stavros-k
Copy link
Collaborator

Meh, I've ever seen only 3-4 apps requiring .env file to be there and writable...

A solution is to edit the dockerfile on our mirror and create an empty .env file.

Configmap would be read-only.
Mounting the whole /app dir will probably cause problems later.

@truecharts-admin truecharts-admin added precommit:ok CI status: pre-commit validation successful lint:ok CI status: linting successful install:ok CI status: install successful labels Jul 2, 2022
@PrivatePuffin
Copy link
Member

cant we just chown the log dir using an initscript and keep it on 568?

@stavros-k
Copy link
Collaborator

cant we just chown the log dir using an initscript and keep it on 568?

Probably not. if I remember right, the user running the process that writes on logs dir is www-data

@schnerring
Copy link
Contributor Author

schnerring commented Jul 2, 2022

Mounting the whole /app dir will probably cause problems later.

Yes, it's what caused #3006

A solution is to edit the dockerfile on our mirror and create an empty .env file.

The empty file is actually there, but only writable by root. I also tried setting PUID to 0 but the web process always runs as www-data user.

Probably not. if I remember right, the user running the process that writes on logs dir is www-data

The same goes for the backup dir.

@stavros-k
Copy link
Collaborator

The empty file is actually there, but only writable by root. I also tried setting PUID to 0 but the web process always runs as www-data user.

What happens if you run it with user 0, group 82 and fsgroup 82 ?

@schnerring
Copy link
Contributor Author

What happens if you run it with user 0, group 82 and fsgroup 82 ?

Like this?

image

Still getting the same error:

The stream or file "/app/storage/logs/laravel-2022-07-02.log" could not be opened in append mode: Failed to open stream: Permission denied

@stavros-k
Copy link
Collaborator

What happens if you run it with user 0, group 82 and fsgroup 82 ?

Like this?

image

Still getting the same error:

The stream or file "/app/storage/logs/laravel-2022-07-02.log" could not be opened in append mode: Failed to open stream: Permission denied

fsGroup to 82 also

@schnerring
Copy link
Contributor Author

fsGroup to 82 also

That works, too, just as my fix with (0, 0, 82). What are you hoping to achieve with this?

Here's the ps a output if that's helpful:

/app # ps a
PID   USER     TIME  COMMAND
    1 root      0:00 {supervisord} /usr/bin/python3 /usr/bin/supervisord -c /etc/supervisor.d/supervisord.ini
    7 root      0:00 php-fpm: master process (/usr/local/etc/php-fpm.conf)
    8 root      0:00 nginx: master process /usr/sbin/nginx -g daemon off;
    9 www-data  0:00 php-fpm: pool www
   10 www-data  0:00 php-fpm: pool www
   11 nginx     0:00 nginx: worker process
   12 nginx     0:00 nginx: worker process
   13 nginx     0:00 nginx: worker process
   14 nginx     0:00 nginx: worker process
   15 nginx     0:00 nginx: worker process
   16 nginx     0:00 nginx: worker process
   17 nginx     0:00 nginx: worker process
   18 nginx     0:00 nginx: worker process
   19 root      0:00 /bin/sh
   27 root      0:00 /bin/sh
   33 root      0:00 ps a

@stavros-k
Copy link
Collaborator

fsGroup to 82 also

That works, too, just as my fix with (0, 0, 82). What are you hoping to achieve with this?

Here's the ps a output if that's helpful:

/app # ps a
PID   USER     TIME  COMMAND
    1 root      0:00 {supervisord} /usr/bin/python3 /usr/bin/supervisord -c /etc/supervisor.d/supervisord.ini
    7 root      0:00 php-fpm: master process (/usr/local/etc/php-fpm.conf)
    8 root      0:00 nginx: master process /usr/sbin/nginx -g daemon off;
    9 www-data  0:00 php-fpm: pool www
   10 www-data  0:00 php-fpm: pool www
   11 nginx     0:00 nginx: worker process
   12 nginx     0:00 nginx: worker process
   13 nginx     0:00 nginx: worker process
   14 nginx     0:00 nginx: worker process
   15 nginx     0:00 nginx: worker process
   16 nginx     0:00 nginx: worker process
   17 nginx     0:00 nginx: worker process
   18 nginx     0:00 nginx: worker process
   19 root      0:00 /bin/sh
   27 root      0:00 /bin/sh
   33 root      0:00 ps a

To make the .env writable :D

@schnerring
Copy link
Contributor Author

Nope, same issue:

/app # ls -l .env 
-rw-r--r--    1 root     root           644 Jun 10 17:31 .env

@stavros-k
Copy link
Collaborator

Nope, same issue:

/app # ls -l .env 
-rw-r--r--    1 root     root           644 Jun 10 17:31 .env

Well, in that case and considering the .env is already present in the container. It might be better to open a bug upstream that they should chown the file in their Dockerfile.

@stavros-k stavros-k merged commit 91ea576 into truecharts:master Jul 2, 2022
@schnerring
Copy link
Contributor Author

schnerring commented Jul 2, 2022

Well, in that case and considering the .env is already present in the container. It might be better to open a bug upstream that they should chown the file in their Dockerfile.

I agree that this is an upstream issue, so I opened one:

Kovah/LinkAce#499

If I understand correctly this is a limitation of the first-time setup wizard which is probably more complicated to implement than your idea to modify our mirror image. I think the change is small enough, improves usability, and possible reduces the number of support tickets coming your way.

@schnerring schnerring deleted the fix-linkace branch July 2, 2022 21:18
@stavros-k
Copy link
Collaborator

Now I remember why I was familiar with this .env file problems....
Kovah/LinkAce#398

@truecharts-admin
Copy link
Collaborator

This PR is locked to prevent necro-posting on closed PRs. Please create a issue or contact staff on discord if you want to further discuss this

@truecharts truecharts locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
install:ok CI status: install successful lint:ok CI status: linting successful precommit:ok CI status: pre-commit validation successful size/XS Categorises a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants